Skip to content

Lookup altitude #1518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Aug 31, 2022
Merged

Lookup altitude #1518

merged 17 commits into from
Aug 31, 2022

Conversation

nicomt
Copy link
Contributor

@nicomt nicomt commented Aug 9, 2022

  • Closes Altitude lookup table #1516
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`1516` or this Pull Request with :pull:`1518`. Includes contributor name and/or GitHub username (link with :ghuser:`nicomt`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

To make it easier to review, this pull request doesn't replace the default altitude in the Location class or the get_solarposition yet.
I will make another pull request once this one is merged.
One change to note is _degrees_to_index; I simplified the function. I think a lot of the centering and floating point logic is unnecessary. I even compared the output over several thousands of random integers, and their outputs were identical.
Let me know if you rather I change it back.

I changed it back to the original after I realized my new implementation had a bug at the boundaries.

@nicomt
Copy link
Contributor Author

nicomt commented Aug 14, 2022

FYI, I want to be more transparent about how I generated the altitude dataset. I'm trying to consolidate the generation into one python script so is easy for you guys to run it. I will probably release it as a Github Gist soon.

Also, I want to do a better job quantifying the accuracy of the generated map. I will try to find a good source of ground truth for altitudes and release some error stats.

nicomt and others added 5 commits August 17, 2022 23:50
@nicomt
Copy link
Contributor Author

nicomt commented Aug 18, 2022

Here is the Gist I promised that allows you guys to reproduce the altitude map.
https://gist.github.com/nicomt/d2c5f08a8ee3500550be42d8dbee6c1d

While compiling the script, I realized that the lower altitude bound in my first commit was lower than the lowest place on earth (The Death Sea). In this version, I fixed the lower bound to -450m, which removes those bad values and allows me to reduce the step 28m instead of 35m.

Also, I compared the results with a few samples from the Google Elevation API, and here is what I got.

error_diff

It looks like the error is pretty bad in Greenland and Asia but relatively okay elsewhere.
Here are the actual results of my test.

Metric Error
Mean Absolute Error 113
Max Error 3067
Mean Squared Error 133069

Let me know if there is something else I can do to help.

Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test should move to test_tools.py:

def test_degrees_to_index_1():

Actually I don't understand why the test_clearsky.py didn't generate a failure.

nicomt and others added 5 commits August 21, 2022 14:51
@nicomt nicomt requested a review from cwhanse August 21, 2022 22:05
@cwhanse cwhanse added this to the 0.9.3 milestone Aug 22, 2022
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this great PR @nicomt! I was able to successfully run your generation script using the requirements file (conda create -n altmap python=3.8 --file reqs.txt -c conda-forge) and the output I got has the same md5 hash as the one in this PR 👍 LGTM other than the merge conflict.

It looks like the error is pretty bad in Greenland and Asia but relatively okay elsewhere.

At least for Greenland, maybe caused by whether the altitude is reported for the top of the ice sheet versus the underlying bedrock (tilezen/joerd#103)? I'd say let's not worry about it; anyone trying to build a solar farm on an ice sheet will be too distracted by other problems to complain about that error.

@mikofski
Copy link
Member

Thanks so much Nicolas, Two very minor nits that aren't blockers and can be improvements in future PR's, I don't want to hold up merging this asap.

  1. in test_lookup_altitude() a fixture might work better instead of looping over test_locations
  2. in location.lookup_altitude() it would be nice if we could adopt pathlib.Path and the / operator for all new code and retire osp.join(). EG:
    import pathlib
    pvlib_path = pathlib.Path(__file__).parent
    filepath = pvlib_path / 'data' / 'Altitude.h5'

@mikofski mikofski merged commit ac2cb4b into pvlib:master Aug 31, 2022
@nicomt
Copy link
Contributor Author

nicomt commented Aug 31, 2022

@mikofski I will fix those in the next PR. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Altitude lookup table
4 participants